refactor: [M3-8877] - Refactor VPC Create to use react-hook-form
#11357
Conversation
coliu-akamai
changed the title
react-hook-form
coliu-akamai
force-pushed
the
refactor-vpc-create
branch
from
69bf2d4 to
e06832c
Compare
| form.setError('root.subnetLabel', { message: error.reason }); | ||
| } else if (error?.field === 'subnets.ipv4') { | ||
| form.setError('root.subnetIPv4', { message: error.reason }); |
There was a problem hiding this comment.
the API returns subnet general errors as subnet.label or subnet.ipv4. Just setting form.setError(error?.field ...) for this leads to type errors when accessing the formstate's errors, so now using root.xxx. I think doing this is inline with react-hook-form's docs tho! - "you can set a server or global error with root as the key"
There was a problem hiding this comment.
This makes sense to me. Handling API errors like this is hard and I think you did the best we could here.
I was thinking maybe we could use errors.subnets?.root?.message but maybe not because we need to separate states for label and ipv4?
There was a problem hiding this comment.
I got a type error for form.setError('subnets.root', {...}} (but not when using errors.subnets?.root?.message, which I'm a bit confused about 😅)...
I think you're right though, errors.subnets?.root?.message wouldn't help with getting the two separate errors to show up at the same time - I ran into that issue when trying form.setError('subnets', {...}} and errors.subnets?.message. Ended up going with root.xxx
|
|
||
| React.useEffect(() => { | ||
| if (!isEmpty(errors) && submitCount > previousSubmitCount.current) { | ||
| scrollErrorIntoView(undefined, { behavior: 'smooth' }); |
There was a problem hiding this comment.
I'm having trouble with getting the error to scroll for scrollErrorIntoViewV2, so switching back to scrollErrorIntoView for now. Gonna continue poking at it, but putting this up for review since everything else should be ready
| const ipValidationErrorMessage1 = | ||
| 'A subnet must have either IPv4 or IPv6, or both, but not neither.'; |
There was a problem hiding this comment.
This error now surfaces bc of how the VPC's validation schema is set up. However, wondering if it's better to match the original erroring behavior, especially since we don't support ipv6 in VPCs currently - ?
update: adding a temporary IPv4 required message (+ todo comments to change back when IPv6 is supported) to hide mention of IPv6, since we don't show any IPv6 field/IPv6 related stuff when creating a VPC (felt like it could be confusing that we mention ipv6s)
coliu-akamai
requested review from
jdamore-linode,
bnussman-akamai and
cpathipa
and removed request for
a team
|
Coverage Report: ❌ |
coliu-akamai
added
Add'tl Approval Needed
There was a problem hiding this comment.
Nice work! @coliu-akamai!
confirming the verification steps around create VPC flow.
bnussman-akamai
added
Approved
Cloud Manager UI test results🔺 1 failing test on test run #14 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: yarn cy:run -s "cypress/e2e/core/billing/smoke-billing-activity.spec.ts" |
|||||||||||||||||
|
merging - failing test fixed in dev (thanks Banks!) |
Description 📝
Changes 🔄
Target release date 🗓️
n/a
How to test 🧪
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅